Repository navigation
fix(docker): pre-pull the Dockerfile syntax frontend image - #150
Conversation
![dap-code-review-by-crunchloop[bot]](https://avatars.githubusercontent.com/in/4707266?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review — 2a1be4c · ADVISORY
The pre-pull still misses a BuildKit-supported JSON syntax declaration, so a custom frontend in that form remains subject to the older session-less resolution failure.
| Finding | Anchor | |
|---|---|---|
| 🟡 MEDIUM | JSON syntax directives are not pre-pulled core/correctness |
build.go:402 |
What I reviewed
Coverage: Reviewed commit 2a1be4c14b13cc54ef9d21b52ba96260a27d9a4a (one commit; four changed files). Worked correctness, interface-contract, test-coverage, redelivery (not applicable), and version-compatibility. Traced BuildImage into BuildKit v0.30.0's actual Dockerfile parser and frontend dispatch; checked the new detector against that parser and its parser tests. Also read the repository's engineering review guidance at the governing base SHA: .dap/code-review.yml is absent, so the repository-specific R1–R8 guidance in .dap/review/engineering.md was considered alongside the core dimensions. Build context/cache determinism, lifecycle, path parity, and config flow are unaffected. The integration test covers the comment directive form but not BuildKit's supported JSON syntax form. No further unresolved candidates.
ADVISORY — findings worth reading, none of them blocking. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.
BuildImage builds through ImageBuild with no BuildKit session, and BuildKit before v0.31 (Docker Engine before 29.6) refuses registry access without one, even for anonymous public pulls: a bearer challenge with no session fails with "no active sessions". FROM images were already pre-pulled through the classic pull API for this reason; a `# syntax=` frontend was not, so any user Dockerfile naming one failed before parsing (#148). Pre-pull the frontend too: the BUILDKIT_SYNTAX build arg, else the syntax parser directive, read as BuildKit's parser.DetectSyntax reads it (leading directive lines only, # or // comments, after an optional BOM and shebang; the image is the value's first word). Verified against a Docker 29.5.0 daemon (containerd image store): the new integration test fails there with the reported error without this change and passes with it. Docker 29.6+ resolves anonymously without a session, so it passes there either way. Fixes #148. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2a1be4c to
7a81075
Compare
There was a problem hiding this comment.
Code Review — 7a81075 · ADVISORY
Answer: One existing correctness finding remains open: pre-pull detection still omits a syntax form accepted by BuildKit. I found no additional independent defects in this change.
| Finding | Anchor | |
|---|---|---|
| 🟡 MEDIUM | JSON syntax directives are not pre-pulled core/correctness |
build.go:402 |
What I reviewed
Coverage: Reviewed the single commit at 7a81075b7879e7a999c25321e5266a98559ae819 (four files) for correctness, interface contract, path parity, test coverage, and operability. Verified the frontend-selection behavior against BuildKit v0.30.0 source and traced pre-pulling through the Docker runtime build path. The repository’s governing standard is .dap/review/engineering.md at the directives base SHA; .dap/code-review.yml and AGENTS.md are absent. The added unit cases cover directive parsing and argument precedence, and the integration test covers a parser-directive frontend; JSON syntax input is not covered. No schema or published interface changed, so version compatibility does not apply. No other unreviewed changed behavior remains.
ADVISORY — findings worth reading, none of them blocking. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.
BuildKit's parser.DetectSyntax falls back, after the # and // directive forms, to a JSON object with a string "syntax" member, the input of custom frontends that take JSON, and uses the whole value as the frontend image. syntaxFrontend left that form out, so its frontend was not pre-pulled and hit the session-less resolution failure on engines before 29.6 (DAP review finding 3891 on #150). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review — 8c50252 · CLEAN
Result. At HEAD 8c50252782a02a0ee10be1116876ca0bbb91ea30, the carried correctness finding is fixed: the JSON syntax fallback now detects and pre-pulls the frontend BuildKit selects. I found no additional concrete defects in the new change.
What I reviewed
Coverage. Reviewed the new head commit (2 changed files) and sanity-checked the branch range (3 commits, 17 files); this was a re-review of the new commit, so the earlier lockfile changes were not re-investigated. Checked correctness, the parser behavior contract, and test coverage: compared the implementation with BuildKit v0.30.0 DetectSyntax, followed the call through prePullBaseImages to ImageBuild, and verified unit cases for JSON syntax and precedence. Repository directives were read from the governing SHA: .dap/review/engineering.md; .dap/code-review.yml and AGENTS.md are absent there. No backend behavior beyond the Docker backend was reached; other conditional dimensions do not apply to this parser addition.
CLEAN — this round found nothing. Reply in a finding's own thread to argue it. On the pull request I answer @dap-code-review-by-crunchloop review, pause and resume.
* fix(docker): pre-pull the Dockerfile syntax frontend image (#150) * fix(docker): pre-pull the Dockerfile syntax frontend image BuildImage builds through ImageBuild with no BuildKit session, and BuildKit before v0.31 (Docker Engine before 29.6) refuses registry access without one, even for anonymous public pulls: a bearer challenge with no session fails with "no active sessions". FROM images were already pre-pulled through the classic pull API for this reason; a `# syntax=` frontend was not, so any user Dockerfile naming one failed before parsing (#148). Pre-pull the frontend too: the BUILDKIT_SYNTAX build arg, else the syntax parser directive, read as BuildKit's parser.DetectSyntax reads it (leading directive lines only, # or // comments, after an optional BOM and shebang; the image is the value's first word). Verified against a Docker 29.5.0 daemon (containerd image store): the new integration test fails there with the reported error without this change and passes with it. Docker 29.6+ resolves anonymously without a session, so it passes there either way. Fixes #148. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(docker): pre-pull a frontend named by a JSON syntax document BuildKit's parser.DetectSyntax falls back, after the # and // directive forms, to a JSON object with a string "syntax" member, the input of custom frontends that take JSON, and uses the whole value as the frontend image. syntaxFrontend left that form out, so its frontend was not pre-pulled and hit the session-less resolution failure on engines before 29.6 (DAP review finding 3891 on #150). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: bilby91 <2201079+bilby91@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> * fix(features): enforce lockfile pins on baked features and empty configs (#153) * fix(features): enforce lockfile pins on baked features and empty configs Two findings from the review of #146: - A feature the base image's label satisfied was skipped and its existing lock entry copied, so frozen mode accepted a base image built from another artifact, or one the lockfile had no entry for. The label already records each feature's resolvedRef, but ParseMetadataLabel dropped it (and so did carrying base entries into a rebuilt label). Keep it on FeatureMetadata, read the lockfile before matching the label, and accept a baked feature only when its artifact is the pinned one or, with no pin, a recorded one, which is then locked. Anything else is fetched at its pin and installed. LockfileDisabled keeps the label-only match. - layerFeatures returned before the lockfile when no features were declared, so removing the last feature left its entry behind and frozen mode accepted it. Reconcile an existing lockfile then too: default and upgrade empty it, frozen reports the stale entries. Without a lockfile nothing is created, as with the reference CLI. The pre-baked hot path is unchanged for images this library built, whose labels have recorded resolvedRef since v0.1.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(features): re-resolve baked features in lockfile upgrade mode acceptsBaked found no pin in LockfileUpgrade (pin ignores the lockfile there) and so accepted any recorded baked artifact, which commit then locked: an upgrade could replace a newer pin with the older artifact the base image carries (DAP review finding 3902 on #153). Upgrade mode now accepts no baked feature, so each reference is resolved afresh. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: bilby91 <2201079+bilby91@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> * feat(features): install features reachable only through dependsOn dependsOn only ordered features that devcontainer.json declared anyway: an undeclared dependency was silently skipped, so a feature relying on one installed without it (#147). The spec makes dependsOn a hard dependency, installed recursively with the options it names. - layerFeatures' fetch loop appends each fetched feature's dependsOn requirements and fetches them in turn: lockfile pinning, option validation and defaults apply as for declared features, and they are locked under their dependsOn key as the reference CLI does. - Spec equality decides duplicates: same content (digest) and same options once defaults apply. A repeat of a key with the same options is skipped before fetching, which also ends cycles; the same content under another reference is dropped after. One feature with different options is installed once per set, and topoSort maps an id to all its entries, pointing dependsOn edges at the one with the requested options. - A dependency the base image's label satisfies is not fetched or installed again: bakedFeature, factored from markAlreadyInstalled, includes the lockfile's check of the artifact the label records. Its own dependencies are still followed, as are a declared baked feature's, since an image built before this change lacks them. Labels now record each feature's dependsOn ({} when none); for a label written before that, the baked feature's metadata is fetched at its recorded artifact to find them. - A dependency dropped as a duplicate under another reference is still locked under its own key. - The compose inputs hash also covers a local dependency's content, which the declaring feature's content does not; projects without one hash as before. - OCI references are lowercased before parsing, as the CLI's getRef does: published dependsOn keys such as ghcr.io/codspace/dependson/A failed to parse. The integration test builds the CLI's own lockfile-dependson fixture in frozen mode against its expected lockfile. Closes #147. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(features): record the baked match's option gap for dependencies The DAP review of #151 (finding 3903) notes a baked dependency is reused whatever options it was requested with. The pre-baked match has never compared options, for declared features either; record the limitation in §5.1 and track the fix to the match itself in #154. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(features): lock an unpinned baked feature with its dependsOn The DAP review of #153 (finding 3904) noted a baked feature accepted without a pin was locked from its label entry, which then had no dependsOn. This change fills it in before commit; pin it with a test for a label that records dependsOn and for an older one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(features): confine local dependsOn paths The DAP review of #151 (findings 3912, 3919, 3917) showed a fetched feature's dependsOn could name a local path, absolute or escaping with .., which layerFeatures resolved against devcontainer.json's directory, copied into the build context and ran: published metadata choosing a host directory to execute. The compose inputs hash also missed such a dependency, since it follows local dependencies from declared local features only. A local dependsOn key is now followed only from a local feature, and only as a relative path that resolves, symlinks included, inside the directory of devcontainer.json, as the reference CLI requires of local features; anything else fails Up and Build. With remote features unable to reach local dependencies, the compose hash's traversal covers every one. Declared local features keep their existing resolution. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(features): never take a dependsOn dependency from the base image The DAP review of #151 (finding 3913) showed a dependency the base image's label listed was reused whatever options its dependsOn entry asked for: the pre-baked match compares id and version only, and those options are the feature author's, which no user config could route through that match before. Dependencies are now always fetched and installed; a declared baked feature still is not reinstalled, and its dependencies are installed. Until #154 adds option matching, a pre-baked image whose features have dependencies reinstalls them on every build. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: bilby91 <2201079+bilby91@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #148.
Cause
BuildImagebuilds throughImageBuildwithout a BuildKit session. BuildKit releases before v0.31 (Docker Engine before 29.6) refuse to contact the registry without a session, even for anonymous public pulls. Docker Hub answers with a bearer challenge, and without a session that fails as "no active sessions". The anonymous fallback arrived in moby/buildkiteb866c3d2.FROMimages were already pre-pulled through the classic pull API for exactly this reason. A# syntax=frontend was not, so on older engines any user Dockerfile naming one failed before parsing.Fix
The issue's option 1: the existing pre-pull now also covers the frontend image.
BUILDKIT_SYNTAXbuild arg if set, otherwise from thesyntaxparser directive.parser.DetectSyntaxreads it:#or//comments at column 0;#!line;Options not taken:
Testing
TestSyntaxFrontendhas 15 cases, one per parser rule.TestEngineBuild_BuildSource_SyntaxDirectiveuses# syntax=docker/dockerfile:1withCOPY --chmodandRUN --mount.go vet, lint andgo test -race ./...pass.🤖 Generated with Claude Code